Location conversion tests for relay and parachains #487

x3c41a · 2024-10-17T16:41:22Z

Added location convertion tests.
Polkadot sdk PR
Fixes #488

…matter

acatangiu

The conversions are the same for all system parachains, so this test could be a macro to dedup the code, but I don't know where we would put that macro or if it's worth the effort. I'm fine with the duplicated code too.

Please see the other comments.

acatangiu · 2024-10-21T13:55:27Z

relay/kusama/tests/location_conversion.rs

+		// DescribeTerminus
+		TestCase {
+			description: "DescribeTerminus Parent",
+			location: Location::new(1, Here),
+			expected_account_id_str: "5GyWtDJP7qaipWRGr4KJ6VUDxRXf4jDnPW6KPTeCekHfqZkD",
+		},
+		TestCase {
+			description: "DescribeTerminus Sibling",
+			location: Location::new(1, [Parachain(1111)]),
+			expected_account_id_str: "5EC5GfEFm9XEBYjXzxb1VseMHsG2VhPeGTGWF9H8tYZnGsSk",
+		},
+		// DescribePalletTerminal
+		TestCase {
+			description: "DescribePalletTerminal Parent",
+			location: Location::new(1, [PalletInstance(50)]),
+			expected_account_id_str: "5CnwemvaAXkWFVwibiCvf2EjqwiqBi29S5cLLydZLEaEw6jZ",
+		},
+		TestCase {
+			description: "DescribePalletTerminal Sibling",
+			location: Location::new(1, [Parachain(1111), PalletInstance(50)]),
+			expected_account_id_str: "5GFBgPjpEQPdaxEnFirUoa51u5erVx84twYxJVuBRAT2UP2g",
+		},
+		// DescribeAccountId32Terminal
+		TestCase {
+			description: "DescribeAccountId32Terminal Parent",
+			location: Location::new(
+				1,
+				[AccountId32 { network: None, id: AccountId::from(ALICE).into() }],
+			),
+			expected_account_id_str: "5DN5SGsuUG7PAqFL47J9meViwdnk9AdeSWKFkcHC45hEzVz4",
+		},
+		TestCase {
+			description: "DescribeAccountId32Terminal Sibling",
+			location: Location::new(
+				1,
+				[
+					Parachain(1111),
+					Junction::AccountId32 { network: None, id: AccountId::from(ALICE).into() },
+				],
+			),
+			expected_account_id_str: "5DGRXLYwWGce7wvm14vX1Ms4Vf118FSWQbJkyQigY2pfm6bg",
+		},
+		// DescribeAccountKey20Terminal
+		TestCase {
+			description: "DescribeAccountKey20Terminal Parent",
+			location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),
+			expected_account_id_str: "5F5Ec11567pa919wJkX6VHtv2ZXS5W698YCW35EdEbrg14cg",
+		},
+		TestCase {
+			description: "DescribeAccountKey20Terminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), AccountKey20 { network: None, key: [0u8; 20] }],
+			),
+			expected_account_id_str: "5CB2FbUds2qvcJNhDiTbRZwiS3trAy6ydFGMSVutmYijpPAg",
+		},
+		// DescribeTreasuryVoiceTerminal
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Treasury, part: BodyPart::Voice }]),
+			expected_account_id_str: "5CUjnE2vgcUCuhxPwFoQ5r7p1DkhujgvMNDHaF2bLqRp4D5F",
+		},
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Treasury, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5G6TDwaVgbWmhqRUKjBhRRnH4ry9L9cjRymUEmiRsLbSE4gB",
+		},
+		// DescribeBodyTerminal
+		TestCase {
+			description: "DescribeBodyTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Unit, part: BodyPart::Voice }]),
+			expected_account_id_str: "5EBRMTBkDisEXsaN283SRbzx9Xf2PXwUxxFCJohSGo4jYe6B",
+		},
+		TestCase {
+			description: "DescribeBodyTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Unit, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5DBoExvojy8tYnHgLL97phNH975CyT45PWTZEeGoBZfAyRMH",
+		},


Parent and Sibling (parachain) locations do not make sense for a Relay chain.

All of these should be replaced with Child (parachain) locations.
(DescribeTerminus Child, DescribePalletTerminal Child, etc)

acatangiu · 2024-10-21T13:55:46Z

relay/polkadot/tests/location_conversion.rs

+		// DescribeTerminus
+		TestCase {
+			description: "DescribeTerminus Parent",
+			location: Location::new(1, Here),
+			expected_account_id_str: "5GyWtDJP7qaipWRGr4KJ6VUDxRXf4jDnPW6KPTeCekHfqZkD",
+		},
+		TestCase {
+			description: "DescribeTerminus Sibling",
+			location: Location::new(1, [Parachain(1111)]),
+			expected_account_id_str: "5EC5GfEFm9XEBYjXzxb1VseMHsG2VhPeGTGWF9H8tYZnGsSk",
+		},
+		// DescribePalletTerminal
+		TestCase {
+			description: "DescribePalletTerminal Parent",
+			location: Location::new(1, [PalletInstance(50)]),
+			expected_account_id_str: "5CnwemvaAXkWFVwibiCvf2EjqwiqBi29S5cLLydZLEaEw6jZ",
+		},
+		TestCase {
+			description: "DescribePalletTerminal Sibling",
+			location: Location::new(1, [Parachain(1111), PalletInstance(50)]),
+			expected_account_id_str: "5GFBgPjpEQPdaxEnFirUoa51u5erVx84twYxJVuBRAT2UP2g",
+		},
+		// DescribeAccountId32Terminal
+		TestCase {
+			description: "DescribeAccountId32Terminal Parent",
+			location: Location::new(
+				1,
+				[AccountId32 { network: None, id: AccountId::from(ALICE).into() }],
+			),
+			expected_account_id_str: "5DN5SGsuUG7PAqFL47J9meViwdnk9AdeSWKFkcHC45hEzVz4",
+		},
+		TestCase {
+			description: "DescribeAccountId32Terminal Sibling",
+			location: Location::new(
+				1,
+				[
+					Parachain(1111),
+					Junction::AccountId32 { network: None, id: AccountId::from(ALICE).into() },
+				],
+			),
+			expected_account_id_str: "5DGRXLYwWGce7wvm14vX1Ms4Vf118FSWQbJkyQigY2pfm6bg",
+		},
+		// DescribeAccountKey20Terminal
+		TestCase {
+			description: "DescribeAccountKey20Terminal Parent",
+			location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),
+			expected_account_id_str: "5F5Ec11567pa919wJkX6VHtv2ZXS5W698YCW35EdEbrg14cg",
+		},
+		TestCase {
+			description: "DescribeAccountKey20Terminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), AccountKey20 { network: None, key: [0u8; 20] }],
+			),
+			expected_account_id_str: "5CB2FbUds2qvcJNhDiTbRZwiS3trAy6ydFGMSVutmYijpPAg",
+		},
+		// DescribeTreasuryVoiceTerminal
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Treasury, part: BodyPart::Voice }]),
+			expected_account_id_str: "5CUjnE2vgcUCuhxPwFoQ5r7p1DkhujgvMNDHaF2bLqRp4D5F",
+		},
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Treasury, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5G6TDwaVgbWmhqRUKjBhRRnH4ry9L9cjRymUEmiRsLbSE4gB",
+		},
+		// DescribeBodyTerminal
+		TestCase {
+			description: "DescribeBodyTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Unit, part: BodyPart::Voice }]),
+			expected_account_id_str: "5EBRMTBkDisEXsaN283SRbzx9Xf2PXwUxxFCJohSGo4jYe6B",
+		},
+		TestCase {
+			description: "DescribeBodyTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Unit, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5DBoExvojy8tYnHgLL97phNH975CyT45PWTZEeGoBZfAyRMH",
+		},


Parent and Sibling (parachain) locations do not make sense for a Relay chain.

All of these should be replaced with Child (parachain) locations.
(DescribeTerminus Child, DescribePalletTerminal Child, etc)

acatangiu · 2024-10-21T13:58:18Z

system-parachains/asset-hubs/asset-hub-kusama/tests/tests.rs

+				[AccountId32 {
+					network: None,
+					id: polkadot_core_primitives::AccountId::from(ALICE).into(),
+				}],


You can dedup at least these "Alice" AccountIds used throughout the test:

Suggested change

[AccountId32 {

network: None,

id: polkadot_core_primitives::AccountId::from(ALICE).into(),

}],

[alice.clone()],

and have at the beginning of the test a

let alice = AccountId32 { network: None, id: polkadot_core_primitives::AccountId::from(ALICE).into(), };

acatangiu · 2024-10-21T14:01:15Z

system-parachains/asset-hubs/asset-hub-kusama/tests/tests.rs

+		// DescribeAccountKey20Terminal
+		TestCase {
+			description: "DescribeAccountKey20Terminal Parent",
+			location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),


Same for these AccountKey20s:

Suggested change

location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),

location: Location::new(1, [bob_20.clone()]),

with

let bob_20 = AccountKey20 { network: None, key: [123u8; 20] }

(notice I also suggested using non-zero keys)

☑️
Out of curiosity, what edge-case you want to avoid by eliminating zero keys?

x3c41a · 2024-10-23T10:53:34Z

bot fmt

x3c41a · 2024-10-23T10:54:50Z

The conversions are the same for all system parachains, so this test could be a macro to dedup the code, but I don't know where we would put that macro or if it's worth the effort. I'm fine with the duplicated code too.

Please see the other comments.

I don't feel like writing macros just yet, still want to get used to Rust.

…implement Copy trait

x3c41a added 3 commits October 17, 2024 16:27

added location conversion to relays and parachains

01ffdc2

added test macro and formatted the crate

2e0bc1d

added missing test macro and formatted test files

aa53f0e

x3c41a marked this pull request as draft October 17, 2024 17:03

x3c41a added 3 commits October 17, 2024 20:25

reverted unintended fmt changes caused by wrong version of nighly for…

438c935

…matter

Merge branch 'main' into loc_conv_test

9149704

removed an extra space

b94f2a2

x3c41a marked this pull request as ready for review October 17, 2024 20:36

x3c41a marked this pull request as draft October 17, 2024 20:36

x3c41a changed the title ~~Loc conv test~~ Location conversion tests for relay and parachains Oct 18, 2024

x3c41a added 2 commits October 18, 2024 11:59

modified changelog

2c0661a

Merge branch 'main' into loc_conv_test

ccb601d

x3c41a marked this pull request as ready for review October 18, 2024 09:47

acatangiu suggested changes Oct 21, 2024

View reviewed changes

x3c41a added 4 commits October 23, 2024 13:40

changed relay conversion tests

e381820

refactored system-parachain tests

ce255ad

removed extra namespace xcm::prelude::

331567a

Merge branch 'main' into loc_conv_test

e8c9813

x3c41a requested a review from acatangiu October 23, 2024 11:11

x3c41a marked this pull request as draft October 23, 2024 11:37

x3c41a added 2 commits October 23, 2024 14:53

formatted the change

ef05ebb

clippy: removed AccountId32 and AccountKey20 cloning since they both …

61c44d6

…implement Copy trait

x3c41a marked this pull request as ready for review October 23, 2024 15:06

acatangiu approved these changes Oct 24, 2024

View reviewed changes

franciscoaguirre approved these changes Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Location conversion tests for relay and parachains #487

Location conversion tests for relay and parachains #487

x3c41a commented Oct 17, 2024 •

edited

Loading

acatangiu left a comment

acatangiu Oct 21, 2024

x3c41a Oct 23, 2024

acatangiu Oct 21, 2024

x3c41a Oct 23, 2024

acatangiu Oct 21, 2024

x3c41a Oct 23, 2024

acatangiu Oct 21, 2024

x3c41a Oct 23, 2024

x3c41a commented Oct 23, 2024

x3c41a commented Oct 23, 2024

	location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),
	location: Location::new(1, [bob_20.clone()]),

Location conversion tests for relay and parachains #487

Are you sure you want to change the base?

Location conversion tests for relay and parachains #487

Conversation

x3c41a commented Oct 17, 2024 • edited Loading

acatangiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x3c41a commented Oct 23, 2024

x3c41a commented Oct 23, 2024

x3c41a commented Oct 17, 2024 •

edited

Loading